Implement new `get_or_undefined` method for `[JsValue]` #1492

jedel1043 · 2021-08-22T05:42:47Z

This Pull Request fixes/closes #1490.

It changes the following:

Adds a new trait JsArgs to add new methods to [JsValue]
Implements a new method get_or_undefined for [JsValue]
Replaces calls to args.get(n).cloned().unwrap_or_default() with args.get_or_undefined()

This should improve the usability of the api by not having to unwrap_or_default everytime we want an argument inside a builtin function.

Razican · 2021-08-23T10:41:25Z

Hmmm I'm not sure if the extra trait and code improves the ergonomic enough to stoping using well known standard library APIs and adding a new trait.

I would like to know the thoughts of more people.

RageKnify · 2021-08-23T13:48:19Z

If we are to do something like this I think returning a reference would be better, that way we would be reducing the number of clones.
To get the reference to undefined it might work to have a constant? Not sure if it's possible, but if so I think it'd be a nice solution.
We'd have const undefined: Value = Value::Undefined; in the trait's file and then we can use a reference to it in the method.

jedel1043 · 2021-08-23T14:23:32Z

Hmmm I'm not sure if the extra trait and code improves the ergonomic enough to stoping using well known standard library APIs and adding a new trait.

Completely understandable. I just wanted to point out that you can still use get as always without any problem; there are a lot of uses of get without defaulting to undefined. This is just a utility function to simplify a common action that appears 60+ times in the codebase.

jedel1043 · 2021-08-23T14:26:40Z

If we are to do something like this I think returning a reference would be better, that way we would be reducing the number of clones.
To get the reference to undefined it might work to have a constant? Not sure if it's possible, but if so I think it'd be a nice solution.
We'd have const undefined: Value = Value::Undefined; in the trait's file and then we can use a reference to it in the method.

Let me try this. I tried to use a Cow but the performance was worse than cloning, so i opted for the clone. A const if its possible or a static if it isn't should do the trick.

Razican · 2021-08-25T06:14:10Z

┌─────────┬──────────────────────────────────────────────┬────────────────────┬─────────────────────┬────────────┐
│ (index) │                     name                     │  changesDuration   │   masterDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼────────────────────┼─────────────────────┼────────────┤
│    0    │     'Arithmetic operations (Execution)'      │   '228.6±0.33ns'   │ '**221.5±0.46ns**'  │   '+3.0'   │
│    1    │        'Arithmetic operations (Full)'        │ '**263.7±0.46µs**' │   '266.1±0.49µs'    │  '-0.99'   │
│    2    │          'Array access (Execution)'          │  '**5.0±0.02µs**'  │    '5.8±0.02µs'     │   '-13'    │
│    3    │            'Array access (Full)'             │   '288.5±0.49µs'   │   '287.3±0.50µs'    │   '0.0'    │
│    4    │         'Array creation (Execution)'         │  '**2.2±0.01ms**'  │    '2.7±0.00ms'     │   '-18'    │
│    5    │           'Array creation (Full)'            │    '2.9±0.00ms'    │  '**2.7±0.00ms**'   │   '+10'    │
│    6    │           'Array pop (Execution)'            │ '**816.0±2.49µs**' │   '863.8±2.42µs'    │   '-5.7'   │
│    7    │              'Array pop (Full)'              │  '1280.0±1.12µs'   │ '**1213.2±1.42µs**' │   '+6.0'   │
│    8    │     'Boolean Object Access (Execution)'      │  '**4.6±0.01µs**'  │    '4.6±0.01µs'     │  '-0.99'   │
│    9    │        'Boolean Object Access (Full)'        │   '282.8±0.35µs'   │   '281.7±0.36µs'    │   '0.0'    │
│   10    │            'Clean js (Execution)'            │ '**624.6±3.57µs**' │   '637.9±3.52µs'    │   '-2.0'   │
│   11    │              'Clean js (Full)'               │   '939.3±3.08µs'   │ '**930.7±3.05µs**'  │   '+1.0'   │
│   12    │             'Clean js (Parser)'              │ '**31.0±0.04µs**'  │    '31.3±0.15µs'    │  '-0.99'   │
│   13    │                'Create Realm'                │ '**312.4±0.51ns**' │   '349.6±1.77ns'    │   '-11'    │
│   14    │ 'Dynamic Object Property Access (Execution)' │  '**3.9±0.02µs**'  │    '4.5±0.01µs'     │   '-13'    │
│   15    │   'Dynamic Object Property Access (Full)'    │   '285.8±0.44µs'   │   '286.2±1.20µs'    │   '0.0'    │
│   16    │            'Expression (Parser)'             │    '5.2±0.01µs'    │    '5.2±0.02µs'     │   '0.0'    │
│   17    │           'Fibonacci (Execution)'            │   '687.8±2.98µs'   │ '**677.5±0.67µs**'  │   '+2.0'   │
│   18    │              'Fibonacci (Full)'              │   '963.8±1.72µs'   │   '967.6±2.30µs'    │   '0.0'    │
│   19    │            'For loop (Execution)'            │ '**15.1±0.06µs**'  │    '17.0±0.07µs'    │   '-12'    │
│   20    │              'For loop (Full)'               │ '**290.3±1.53µs**' │   '294.1±0.37µs'    │  '-0.99'   │
│   21    │             'For loop (Parser)'              │   '14.7±0.05µs'    │    '14.8±0.01µs'    │   '0.0'    │
│   22    │           'Goal Symbols (Parser)'            │ '**10.6±0.01µs**'  │    '10.6±0.01µs'    │  '-0.99'   │
│   23    │            'Hello World (Parser)'            │    '3.0±0.02µs'    │    '3.0±0.01µs'     │   '0.0'    │
│   24    │             'Long file (Parser)'             │   '729.1±1.47ns'   │   '730.3±9.76ns'    │   '0.0'    │
│   25    │            'Mini js (Execution)'             │ '**572.2±4.35µs**' │   '585.1±3.88µs'    │   '-2.0'   │
│   26    │               'Mini js (Full)'               │   '885.2±2.57µs'   │ '**873.6±2.65µs**'  │   '+1.0'   │
│   27    │              'Mini js (Parser)'              │ '**27.4±0.03µs**'  │    '27.6±0.03µs'    │  '-0.99'   │
│   28    │      'Number Object Access (Execution)'      │    '3.7±0.01µs'    │    '3.6±0.01µs'     │   '0.0'    │
│   29    │        'Number Object Access (Full)'         │ '**276.3±0.34µs**' │   '277.9±0.43µs'    │  '-0.99'   │
│   30    │        'Object Creation (Execution)'         │  '**4.0±0.01µs**'  │    '4.0±0.02µs'     │  '-0.99'   │
│   31    │           'Object Creation (Full)'           │   '279.6±0.26µs'   │   '280.5±0.48µs'    │   '0.0'    │
│   32    │             'RegExp (Execution)'             │   '11.0±0.06µs'    │    '10.9±0.06µs'    │   '0.0'    │
│   33    │               'RegExp (Full)'                │ '**285.2±0.35µs**' │   '287.4±0.32µs'    │  '-0.99'   │
│   34    │         'RegExp Literal (Execution)'         │   '11.0±0.06µs'    │    '11.0±0.06µs'    │   '0.0'    │
│   35    │           'RegExp Literal (Full)'            │ '**292.2±1.80µs**' │   '294.3±0.30µs'    │  '-0.99'   │
│   36    │    'RegExp Literal Creation (Execution)'     │  '**7.2±0.03µs**'  │    '8.0±0.02µs'     │   '-11'    │
│   37    │       'RegExp Literal Creation (Full)'       │   '284.9±0.63µs'   │   '284.8±0.34µs'    │   '0.0'    │
│   38    │ 'Static Object Property Access (Execution)'  │  '**3.7±0.01µs**'  │    '4.2±0.02µs'     │   '-12'    │
│   39    │    'Static Object Property Access (Full)'    │   '281.3±0.53µs'   │   '281.0±0.50µs'    │   '0.0'    │
│   40    │      'String Object Access (Execution)'      │    '6.1±0.01µs'    │    '6.1±0.03µs'     │   '0.0'    │
│   41    │        'String Object Access (Full)'         │   '285.1±0.55µs'   │   '283.7±0.38µs'    │   '0.0'    │
│   42    │       'String comparison (Execution)'        │  '**5.3±0.01µs**'  │    '5.8±0.03µs'     │   '-9.9'   │
│   43    │          'String comparison (Full)'          │ '**281.2±1.45µs**' │   '287.0±2.68µs'    │   '-2.0'   │
│   44    │      'String concatenation (Execution)'      │  '**4.0±0.02µs**'  │    '4.5±0.02µs'     │   '-11'    │
│   45    │        'String concatenation (Full)'         │ '**275.9±2.82µs**' │   '280.3±0.44µs'    │   '-2.0'   │
│   46    │          'String copy (Execution)'           │  '**3.1±0.01µs**'  │    '3.6±0.01µs'     │   '-12'    │
│   47    │             'String copy (Full)'             │ '**270.9±0.35µs**' │   '273.5±0.44µs'    │  '-0.99'   │
│   48    │            'Symbols (Execution)'             │  '**2.7±0.03µs**'  │    '3.0±0.01µs'     │   '-9.1'   │
│   49    │               'Symbols (Full)'               │ '**261.7±0.55µs**' │   '264.0±2.52µs'    │  '-0.99'   │
│   50    │                      ''                      │     undefined      │      undefined      │   '+NaN'   │
└─────────┴──────────────────────────────────────────────┴────────────────────┴─────────────────────┴────────────┘

Razican

I'm OK with the changes :) I though we might have a performance improvement, but not really.

jedel1043 · 2021-08-25T06:18:41Z

I'm OK with the changes :) I though we might have a performance improvement, but not really.

Still, there's a lot of small improvements in performance. The last version using clone was a lot slower.

raskad

Looks good!

jedel1043 changed the title ~~Implement new get_or_undefined method for [JsValue] (Clone version)~~ Implement new get_or_undefined method for [JsValue] (Clone version) Aug 22, 2021

jedel1043 changed the title ~~Implement new get_or_undefined method for [JsValue] (Clone version)~~ Implement new get_or_undefined method for [JsValue] Aug 22, 2021

jedel1043 force-pushed the args_auto_undefined_clone branch from 56f696b to ef48ac9 Compare August 22, 2021 16:32

jedel1043 marked this pull request as ready for review August 22, 2021 17:55

jedel1043 force-pushed the args_auto_undefined_clone branch from ef48ac9 to c708233 Compare August 23, 2021 07:03

Razican requested review from HalidOdat, Razican and RageKnify August 23, 2021 07:32

jedel1043 force-pushed the args_auto_undefined_clone branch 4 times, most recently from 45340ce to 5c96fc0 Compare August 24, 2021 21:18

Razican approved these changes Aug 25, 2021

View reviewed changes

jedel1043 force-pushed the args_auto_undefined_clone branch from 5c96fc0 to 6252b9b Compare August 25, 2021 11:37

Razican added API enhancement New feature or request labels Aug 25, 2021

Razican added this to the v0.13.0 milestone Aug 25, 2021

jedel1043 force-pushed the args_auto_undefined_clone branch from 6252b9b to 96d7b6a Compare August 26, 2021 05:12

jedel1043 added 5 commits August 27, 2021 10:26

Create JsArgs trait

9d45527

Implement trait JsArgs for [JsValue]

10e8052

Replace calls to get/unwrap_or with get_or_undefined

7984a92

Return &JsValue from get_or_undefined instead of cloning

f4acddd

Replace more instances and document function

4535aa2

jedel1043 force-pushed the args_auto_undefined_clone branch from 96d7b6a to 4535aa2 Compare August 27, 2021 15:47

jedel1043 requested a review from raskad September 6, 2021 22:11

raskad approved these changes Sep 6, 2021

View reviewed changes

raskad merged commit 2a8d343 into boa-dev:master Sep 6, 2021

jedel1043 deleted the args_auto_undefined_clone branch September 6, 2021 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new `get_or_undefined` method for `[JsValue]` #1492

Implement new `get_or_undefined` method for `[JsValue]` #1492

jedel1043 commented Aug 22, 2021 •

edited

Loading

Razican commented Aug 23, 2021

RageKnify commented Aug 23, 2021

jedel1043 commented Aug 23, 2021

jedel1043 commented Aug 23, 2021

Razican commented Aug 25, 2021

Razican left a comment

jedel1043 commented Aug 25, 2021

raskad left a comment

Implement new get_or_undefined method for [JsValue] #1492

Implement new get_or_undefined method for [JsValue] #1492

Conversation

jedel1043 commented Aug 22, 2021 • edited Loading

Razican commented Aug 23, 2021

RageKnify commented Aug 23, 2021

jedel1043 commented Aug 23, 2021

jedel1043 commented Aug 23, 2021

Razican commented Aug 25, 2021

Razican left a comment

Choose a reason for hiding this comment

jedel1043 commented Aug 25, 2021

raskad left a comment

Choose a reason for hiding this comment

Implement new `get_or_undefined` method for `[JsValue]` #1492

Implement new `get_or_undefined` method for `[JsValue]` #1492

jedel1043 commented Aug 22, 2021 •

edited

Loading